Cleanup PcapLiveDevice synchronous capture.
#2026
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2026 +/- ##
==========================================
- Coverage 83.46% 83.44% -0.02%
==========================================
Files 311 311
Lines 54574 54591 +17
Branches 11535 11820 +285
==========================================
+ Hits 45551 45556 +5
+ Misses 7839 7836 -3
- Partials 1184 1199 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const int64_t timeoutMs = timeout * 1000; // timeout unit is seconds, let's change it to milliseconds | ||
| // A valid timeout is only generated when timeout is positive. | ||
| // This means that the timeout timepoint should be after the start time. | ||
| const bool hasTimeout = timeout > 0; |
There was a problem hiding this comment.
nit: I'd avoid this variable and use timeout > 0 to indicate timeout is used.
I'd also do:
auto timeoutTime = timeout > 0 ? startTime + std::chrono::milliseconds(static_cast<int64_t>(timeout * 1000)) : 0;There was a problem hiding this comment.
I'd avoid this variable and use timeout > 0 to indicate timeout is used.
We can do that, curious as to why? They are both the same thing but the variable is essentially a cached check that you can read better in the code?
auto timeoutTime = timeout > 0 ? startTime + std::chrono::milliseconds(static_cast<int64_t>(timeout * 1000)) : 0;
Timepoint has no constructor from int value, so it would have to be something like:
auto timeoutTime = timeout > 0 ? startTime + std::chrono::milliseconds(static_cast<int64_t>(timeout * 1000)) : std::chrono::steady_clock::time_point();or have it default initialized and then set to an actual value as such:
std::chrono::steady_clock::time_point timeoutTime;
if(timeout > 0) { /* ... */}Timepoint is also a bit verbose to compare to 0 afterwards, as you either have to do timeoutTime > std::chrono::steady_clock::time_point() or timeoutTime.time_since_epoch().count() > 0 to do so.
|
|
||
| if (std::chrono::duration_cast<std::chrono::milliseconds>(currentTime - startTime).count() >= timeoutMs) | ||
| // Check the time only if a valid timeout was specified. Otherwise it would always be true. | ||
| if (hasTimeout && currentTime >= timeoutTime) |
There was a problem hiding this comment.
If we set timeoutTime = 0 if timeout <= 0 we probably don't need hasTimeout here
There was a problem hiding this comment.
If we have timeoutTime = 0 then currentTime >= timeoutTime will always be true, thus always reporting timeout expired even when no timeout was set. We will also have to check timeoutTime > 0 which in the current case is precomputed by the hasTimeout bool?
| try | ||
| { | ||
| if (context->callback(&rawPacket, context->device, context->userCookie)) | ||
| { | ||
| // If the callback returns true, it means that the user wants to stop the capture | ||
| PCPP_LOG_DEBUG("Capture callback requested to stop capturing"); | ||
| context->requestStop = true; | ||
| } | ||
| } | ||
| catch (const std::exception& ex) | ||
| { | ||
| PCPP_LOG_ERROR("Exception occurred while invoking packet arrival callback: " << ex.what()); | ||
| context->requestStop = true; // Stop capture on exception | ||
| } | ||
| catch (...) | ||
| { | ||
| PCPP_LOG_ERROR("Unknown exception occurred while invoking packet arrival callback"); | ||
| context->requestStop = true; // Stop capture on unknown exception | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we decided not to catch exceptions here, no?
There was a problem hiding this comment.
We agreed to not catch exceptions for the other cases because they continued packet capture after catching it. We wanted it to stop which would have been harder to do for the other cases due to running on a separate thread.
In this case, the callback does terminate the packet capture after an exception is caught so its fine. It also solves the unresolved UB that was passing the exceptions through a C compiled code context.
2 participants
Part of #1838
This PR updates the synchronous capture method.